Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove reclaim param for store_accounts_frozen #618

Merged
merged 1 commit into from
Apr 8, 2024
Merged

Remove reclaim param for store_accounts_frozen #618

merged 1 commit into from
Apr 8, 2024

Conversation

HaoranYi
Copy link

@HaoranYi HaoranYi commented Apr 6, 2024 •
edited
Loading

Problem

Follow up #581.

store_frozen_account should never populate reclaims.

Summary of Changes

Remove reclaim param from store_frozen_account.

Fixes #

@HaoranYi HaoranYi changed the title remove reclaim param for store_accounts_frozen Remove reclaim param for store_accounts_frozen Apr 6, 2024
brooksprumo
brooksprumo reviewed Apr 6, 2024
View reviewed changes
Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there still legitimate, non-test, uses for StoreReclaims::Default? Or can we rip out StoreReclaims entirely?

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.8%. Comparing base (de8e9e6) to head (cc95ceb).
Report is 2 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff            @@
##           master     #618     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         851      851             
  Lines      230224   230222      -2     
=========================================
- Hits       188527   188525      -2     
  Misses      41697    41697

@HaoranYi
Copy link
Author

HaoranYi commented Apr 7, 2024 •
edited
Loading

Are there still legitimate, non-test, uses for StoreReclaims::Default? Or can we rip out StoreReclaims entirely?

@brooksprumo
I think so. but it is going to be a bigger PR. We may need to change AccountIndex code too. Need to confirm with @jeffwashington.
It might be better to do that in a different PR? WDYT?

@jeffwashington
Copy link

probably we can rip it out in follow on prs.

jeffwashington
jeffwashington approved these changes Apr 8, 2024
View reviewed changes
Copy link

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

brooksprumo
brooksprumo approved these changes Apr 8, 2024
View reviewed changes
Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@HaoranYi HaoranYi merged commit 287d0d2 into anza-xyz:master Apr 8, 2024
38 checks passed
@HaoranYi HaoranYi deleted the accounts-db/refactor_store_account_frozen branch April 8, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeffwashington jeffwashington jeffwashington approved these changes

@brooksprumo brooksprumo brooksprumo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HaoranYi @codecov-commenter @jeffwashington @brooksprumo